Implement VRF route leaking #160

fedepaol · 2024-06-21T14:48:20Z

Is this a BUG FIX or a FEATURE ?:

Uncomment only one, leave it on its own line:

/kind bug
/kind cleanup
/kind feature
/kind design
/kind flake
/kind failing
/kind documentation
/kind regression

What this PR does / why we need it:

By implementing import VRF, we allow routes defined in the RIB of a router related to a given VRF to be imported from another router tied to another VRF.
This is true for routes being received and being announced.
In case of routes being announced, we still limit the routes to all those defined in the "allowed outgoing" section of a given neighbor.

Special notes for your reviewer:

Release note:

Implement the import vrf feature as described in [https://docs.frrouting.org/en/latest/bgp.html#clicmd-import-vrf-VRFNAME](https://docs.frrouting.org/en/latest/bgp.html#clicmd-import-vrf-VRFNAME). By implementing import VRF, we allow routes defined in the RIB of a router related to a given VRF to be imported from another router tied to another VRF.

karampok · 2024-07-16T11:01:10Z

internal/controller/api_to_config.go

+func validatePrefixes(prefixes []string) error {
+	for _, p := range prefixes {
+		family := ipfamily.ForCIDRString(p)
+		if family == ipfamily.Unknown {


nit: ipfamily.ForCIDRString(p) ==ipfamily.Unknown just because family word is occuring too many times

karampok · 2024-07-16T11:09:39Z

internal/controller/api_to_config.go

@@ -67,8 +67,37 @@ func apiToFRR(resources ClusterResources, alwaysBlock []net.IPNet) (*frr.Config,
 		}

 		alwaysBlockFRR := alwaysBlockToFRR(alwaysBlock)
+		prefixesForVRF := map[string][]string{}


thought: that could be a type X with func newX(routers) X and methods X.validate()

what's "that"? the whole map or []string{} ?

and yes, I agree it's borderline to be readeable

yes I mean the whole map[string][]string but as is also ok

I left the type unchanged but hid the logic behind a couple of functions. Hoping it's readable. I don't feel hiding the map behind a type would be beneficial here.

karampok · 2024-07-16T11:17:45Z

internal/controller/api_to_config.go

+			}
+
+			// All prefixes available in this router: defined and imported from VRFs
+			prefixesInRouter := sets.New(r.Prefixes...)


why pick sets and not using standard library map?

As much as I like sticking to the stdlib, sets are more convenient to get the output ordered (which we want, so the generated configuration is stable).

is the sort the only reason?
but we already have maps for bfd, and sort function in the code base, therefore this introduce different approach

we also use sets already

no I think you are right, here we don't even leverage the order. Let me switch to maps

or not !!! we rely on the order, reverting to set

karampok · 2024-07-16T11:18:26Z

internal/controller/api_to_config.go

@@ -384,6 +400,32 @@ func validateSelectorLengths(mask int, le, ge uint32) error {
 	return nil
 }

+func validateImportVRFs(r v1beta1.Router, allVRFs map[string][]string) error {


is it possible to have go coverage to see how much of this code is being unit-testes?

we can file an issue to add that, or do you think it relates to this PR?

I put random panics and unit-tests are still green, the error branch looks not really unit-tested. But a test coverage would give us precise what we miss.

git diff diff --git a/internal/controller/api_to_config.go b/internal/controller/api_to_config.go index 1b0f7a3..d166f9b 100644 --- a/internal/controller/api_to_config.go +++ b/internal/controller/api_to_config.go @@ -403,9 +403,15 @@ func validateSelectorLengths(mask int, le, ge uint32) error { func validateImportVRFs(r v1beta1.Router, allVRFs map[string][]string) error { for _, i := range r.Imports { if i.VRF == "default" { + if true { + panic("") + } continue } if _, ok := allVRFs[i.VRF]; !ok { + if true { + panic("") + } return fmt.Errorf("router %d-%s imports vrf %s which is not defined", r.ASN, r.VRF, i.VRF) } } @@ -619,6 +625,9 @@ func validatePrefixes(prefixes []string) error { for _, p := range prefixes { family := ipfamily.ForCIDRString(p) if family == ipfamily.Unknown { + if true { + panic("") + } return fmt.Errorf("unknown ipfamily for %s", p) } } [I] kka@f-t14s ~/w/g/m/frr-k8s (leakvrf ✖1)> go test ./internal/controller/... -run=TestConversion/ -count=1 ok github.com/metallb/frr-k8s/internal/controller 0.023s

yep, I understand we are not covering, I am fine with extending but I don't want to add the coverage logic in CI here.

I am not asking to add coverage ci logic, I am only asking if you can see the coverage once locally in your dev machine. The real ask is if you think we should more unit-test and cover allo the error branches that are not covered.

(I think you covered a bit more in the latest push)

I suppose this done (curious though if you can run locally in your dev a coverage, I cannot due to the fact that controller unit test needs envtest way of running)

karampok · 2024-07-16T11:19:29Z

internal/controller/api_to_config.go

+		}
+		for _, p := range n.ToAdvertise.Allowed.Prefixes {
+			if !prefixesInRouter.Has(p) {
+				return fmt.Errorf("trying to advertise non configured prefix %s to neighbor %s, vrf %s", p, neighborName(n.ASN, n.Address), routerConfig.VRF)


log line could split the args into newline.
Also why do we need to error here? I mean that config might makes sense and allow user to filter out per neighbor in the allow prefix? Or is the FRR drops an error?

here we allow to advertise a given prefix to a neighbor. We want to tell the user, he thinks he's allowing a prefix that can't come from anywhere as it's not in the router's prefix or in any imported route from other vrfs.

okay I got it the otherway around :) that might be a feedback. Can I have in allow a prefix a /16 and overtime to add remove /24?

not sure what do you mean by overtime. In general, with allow you must match something you have in hte prefix list (or you can have all).

So I mean on day one user sets on the default vrf the allow prefix list to 10.10.0.0/16, and in day X user adds the prefix in vrf red 10.10.10.0/24 (which is imported) and in day X+1 user in green the prefix 10.10.20.0/24 (which is imported). Is that a use case?

not exactly, as each configuration is meant to be self - contained. So, when you say default imports red, it really means import all the learned routes from red + advertise the prefixes defined locally in the stanza belonging to the red vrf.

karampok · 2024-07-16T12:08:44Z

internal/controller/api_to_config_test.go

+			},
+			err: nil,
+		},
+		{


go test ./internal/controller/... -run=TestConversion/Multiple_Routers_import -v === RUN TestConversion === RUN TestConversion/Multiple_Routers_import_VRFs === RUN TestConversion/Multiple_Routers_import_VRF,_advertise_ips_from_the_imported_vrf === RUN TestConversion/Multiple_Routers_import_VRFs,_advertise --- PASS: TestConversion (0.01s) --- PASS: TestConversion/Multiple_Routers_import_VRFs (0.00s) --- PASS: TestConversion/Multiple_Routers_import_VRF,_advertise_ips_from_the_imported_vrf (0.00s) --- PASS: TestConversion/Multiple_Routers_import_VRFs,_advertise (0.00s) PASS

I cannot tell the diff on the test by looking the description

Unless I am lost in the big file the first and third is the same, and probably the second is superset of the fist

Now it is:

--- PASS: TestConversion/Multiple_Routers_import_VRFs (0.00s) --- PASS: TestConversion/Multiple_Routers_import_VRF,_advertise_ips_from_the_imported_vrf (0.00s) --- PASS: TestConversion/Multiple_Routers_import_non_existing_VRFs (0.00s) --- PASS: TestConversion/Multiple_Routers_import_VRF,_red_imports_default_and_advertises (0.00s)

first is only import with no advertisement. The ones with advertisements are supersets, but I think it still make sense to have them.

karampok · 2024-07-16T12:42:30Z

internal/controller/api_to_config.go

+			// All prefixes available in this router: defined and imported from VRFs
+			prefixesInRouter := sets.New(r.Prefixes...)
+			for _, i := range r.Imports {
+				importedPrefixes := prefixesForVRF[i.VRF]


That is confusing e.g. assigning when the key is not there.
Maybe it be something like

key=i.VRF if i.VRF="default" {key=""} importePrefixes = prefixesForVRF[key]

what happens if key does exist, should we check if v,ok:= prefixesForVRF[key];!ok{}

can we eliminate the transform from default? (maybe when create the original the "" to be "default")

the problem here is, frr wants "default" when importing, while in the api is cleaner not to bother about "default" VRF if you don't have to deal with VRFs at all. Because of that, I couldn't find a better way to do this translation. I think having a slimmer api has more value over this.

Re - key not existing I need to recheck.

I'll find a way to make it clearer. The missing key can't happen because we validate that imported vrfs exist and we fill a record for each vrf, but I agree it's weak and we should at least log (or even better panic) if the key is not there as we think it cna't happen.

karampok · 2024-07-16T12:44:50Z

internal/controller/api_to_config.go

+				if i.VRF == "default" {
+					importedPrefixes = prefixesForVRF[""]
+				}
+				prefixesInRouter.Insert(importedPrefixes...)


what is the behavior if two vrfs have same prefix?

we'll have the same prefix twice which won't be a problem as they are used to understand what we can / can't advertise

is it one or two lines in the final config? (this can be test entry)

actually not, it's a set. So they'll be squashed

karampok · 2024-07-16T12:57:46Z

internal/frr/testdata/TestMultipleRoutersImportVRFs.golden

+ip prefix-list 192.168.1.2-pl-ipv4 seq 1 deny any
+ipv6 prefix-list 192.168.1.2-pl-ipv4 seq 2 deny any
+
+


Could we have a golden file without extra empty lines?

the problem is not related to the golden file but how the templates are generated. We need to tweak the "-" in the go template to reduce them, but it's not a fight that makes sense to fight in this PR.
I'll file an issue to track it.

sounds good for me

karampok · 2024-07-16T13:00:24Z

internal/ipfamily/ipfamily.go

+	res := []string{}
+	for _, p := range prefixes {
+		family := ForCIDRString(p)
+		if family != familyToFilter {


nit: if ForCIDRString(p)!=familyFilter

karampok · 2024-07-17T06:47:45Z

internal/controller/api_to_config.go

+		}
+		for _, p := range n.ToAdvertise.Allowed.Prefixes {
+			if !prefixesInRouter.Has(p) {
+				return fmt.Errorf("trying to advertise non configured prefix %s to neighbor %s, vrf %s", p, neighborName(n.ASN, n.Address), routerConfig.VRF)


So I mean on day one user sets on the default vrf the allow prefix list to 10.10.0.0/16, and in day X user adds the prefix in vrf red 10.10.10.0/24 (which is imported) and in day X+1 user in green the prefix 10.10.20.0/24 (which is imported). Is that a use case?

karampok · 2024-07-17T06:49:15Z

internal/controller/api_to_config.go

@@ -384,6 +400,32 @@ func validateSelectorLengths(mask int, le, ge uint32) error {
 	return nil
 }

+func validateImportVRFs(r v1beta1.Router, allVRFs map[string][]string) error {


I am not asking to add coverage ci logic, I am only asking if you can see the coverage once locally in your dev machine. The real ask is if you think we should more unit-test and cover allo the error branches that are not covered.

karampok · 2024-07-18T11:42:02Z

internal/controller/api_to_config.go

+				return nil, err
+			}
+
+			routerPrefixes, err := importedPrefixes(r, routersPrefixes)


having routerPrefixes and routersPrefixes and many Prefixes makes the code less readable imo.
Maybe instead localPrefixes, allPrefixes

I would also merge the importedPrefixes and validateImportVRFs and rename the function prefixesToImport

local, err: =getLocalPrefixesIfValid() imported,err:=getPrefixToImportIfValid() local=append(local, imported...)

Names are first random thought while reading the code

I would also merge the importedPrefixes and validateImportVRFs and rename the function prefixesToImport

local, err: =getLocalPrefixesIfValid() imported,err:=getPrefixToImportIfValid() local=append(local, imported...)

Names are first random thought while reading the code

This is quite messy already, and I'd rather keep the validation separate so it's clear we are not hiding much behind the courtains.

having routerPrefixes and routersPrefixes and many Prefixes makes the code less readable imo. Maybe instead localPrefixes, allPrefixes

let me try to find a better name

karampok · 2024-07-18T12:10:55Z

internal/controller/api_to_config.go

@@ -384,6 +400,32 @@ func validateSelectorLengths(mask int, le, ge uint32) error {
 	return nil
 }

+func validateImportVRFs(r v1beta1.Router, allVRFs map[string][]string) error {


(I think you covered a bit more in the latest push)

karampok · 2024-07-18T12:11:14Z

internal/controller/api_to_config_test.go

+			},
+			err: nil,
+		},
+		{


karampok · 2024-07-18T12:15:21Z

internal/controller/api_to_config_test.go

@@ -2493,7 +2767,7 @@ func TestConversion(t *testing.T) {
 			if test.err == nil && err != nil {
 				t.Fatalf("expected no error, got %v", err)
 			}
-			if diff := cmp.Diff(frr, test.expected); diff != "" {
+			if diff := cmp.Diff(frr, test.expected, cmpopts.EquateEmpty()); diff != "" {


is EquateEmpty() needs to be in this commit or
in Conversion unit tests: remove the empty fields from expected commit? (I do not mind staying here if the answer no)

I added as part of this test addition, then I filed a commit for cleaning up the others. I could've gone the other way around (or add this with the extra fields and then clean those in a later commit). Didn't seem too bad :P

karampok · 2024-07-18T12:23:20Z

e2etests/pkg/routes/routes.go

@@ -4,7 +4,6 @@ package routes

 import (


About the commit message:
is there a them for the commit? like : word before : can be E2E or API, or Controller ?
maybe E2E: description

note: I did not understood that flag means the return bool value, is it valid term?

karampok · 2024-07-18T12:26:58Z

e2etests/pkg/address/address.go

@@ -0,0 +1,32 @@
+// SPDX-License-Identifier:Apache-2.0


in general the pattern seems to E2E:

what pattern?

sorry, I have been missing the comments that I need to reword before submit the code review.
I am talking about the commit message it has E2E Tests: test, the pattern in previous commits is only E2E:

karampok · 2024-07-18T12:41:08Z

e2etests/tests/import_vrf.go

+
+	defer ginkgo.GinkgoRecover()
+	updater, err := config.NewUpdater()
+	Expect(err).NotTo(HaveOccurred())


this is invalid Expect https://onsi.github.io/ginkgo/#no-assertions-in-container-nodes

in general assign stuff in the tree construction phase is not right
https://onsi.github.io/ginkgo/#mental-model-how-ginkgo-traverses-the-spec-hierarchy

karampok · 2024-07-18T12:41:21Z

e2etests/tests/import_vrf.go

+var _ = ginkgo.Describe("Leaked routes with import vrfs should work", func() {
+	var cs clientset.Interface
+
+	defer ginkgo.GinkgoRecover()


do you know why this is needed?

I probably copy / pasted. Let me remove it and see how it goes.

karampok · 2024-07-18T13:55:36Z

e2etests/pkg/address/address.go

@@ -0,0 +1,32 @@
+// SPDX-License-Identifier:Apache-2.0


sorry, I have been missing the comments that I need to reword before submit the code review.
I am talking about the commit message it has E2E Tests: test, the pattern in previous commits is only E2E:

karampok · 2024-07-18T14:21:00Z

e2etests/tests/import_vrf.go

+
+			updateAndCheckPeered(*config, peersDefault, peersVRF, frrsDefault, frrsVRF, ipFamily)
+
+			ginkgo.By("validating")


this description does add real value

jokes aside, I like seeing a test split in the setup / validation and the By makes both easier to understand while the test is running and when we read it.

It makes the test go from big silence to "I am doing this right now" to knowing where it is, especially when running locally.

In well formed test the BeforeEach/JustBeforeEach is what should be before validating (but table driven kind of make it hard properly form the test because the Before each must have a specific to entry).

Imo running locally the test, you can have prints/delve/It to show the progress.

We can pick it in the issue so no action here

karampok · 2024-07-18T14:21:16Z

e2etests/tests/import_vrf.go

+			frrsDefault, peersDefault, neighborsDefault := initNeighbors(false, ipFamily)
+			frrsVRF, peersVRF, neighborsVRF := initNeighbors(true, ipFamily)
+
+			ginkgo.By("pairing with nodes")


Can be omitted

karampok · 2024-07-18T14:21:57Z

e2etests/tests/import_vrf.go

+			frrsVRF, peersVRF, neighborsVRF := initNeighbors(true, ipFamily)
+
+			ginkgo.By("pairing with nodes")
+			pairWithNodes(frrsDefault, ipFamily, toAdvertise)


I would prefer is all ginkgo asser takes place not nested == function to return error and assert here

karampok · 2024-07-18T14:24:58Z

e2etests/tests/import_vrf.go

+
+			}
+
+			updateAndCheckPeered(*config, peersDefault, peersVRF, frrsDefault, frrsVRF, ipFamily)


passing *config as argument does not look good, is it required?

why not? If the function accepts a value and doesn't expect to modify it, it should accept a pointer. On the other hand, I need to use deepcopy to be able to leverage baseConfig, and that returns a pointer. At some point in the stack I need to reference it. I will do when I get the deepcopy, so it's nicer.

karampok · 2024-07-18T14:31:20Z

e2etests/tests/import_vrf.go

+				}
+				config.Spec.BGP.Routers[0].Neighbors[i].ToReceive.Allowed.Mode = allowMode
+			}
+			for i := range config.Spec.BGP.Routers[1].Neighbors {


here you use range to get the index, imo the thing should work

for _,n:=range xxx{ do something with n }

If it is only to use the array itself a for loop is cleaner.

But the effort to make the first to work (pointers/nested structs etc) might not worth, and therefore keeping as is works for me

if "do something with n" is actually changing n, range won't work

true because https://github.com/metallb/frr-k8s/blob/main/api/v1beta1/frrconfiguration_types.go#L78 is not an array of pointers, was that a design decision?

(no action needed)

why it would be a better design decision for the api?

karampok · 2024-07-19T07:10:21Z

e2etests/pkg/routes/routes.go

@@ -4,7 +4,6 @@ package routes

 import (


karampok · 2024-07-19T07:12:24Z

e2etests/tests/import_vrf.go

+				}
+				config.Spec.BGP.Routers[0].Neighbors[i].ToReceive.Allowed.Mode = allowMode
+			}
+			for i := range config.Spec.BGP.Routers[1].Neighbors {


true because https://github.com/metallb/frr-k8s/blob/main/api/v1beta1/frrconfiguration_types.go#L78 is not an array of pointers, was that a design decision?

(no action needed)

karampok · 2024-07-19T07:16:10Z

e2etests/tests/import_vrf.go

+
+			updateAndCheckPeered(*config, peersDefault, peersVRF, frrsDefault, frrsVRF, ipFamily)
+
+			ginkgo.By("validating")


In well formed test the BeforeEach/JustBeforeEach is what should be before validating (but table driven kind of make it hard properly form the test because the Before each must have a specific to entry).

Imo running locally the test, you can have prints/delve/It to show the progress.

We can pick it in the issue so no action here

karampok · 2024-07-19T07:17:32Z

internal/controller/api_to_config.go

@@ -384,6 +400,32 @@ func validateSelectorLengths(mask int, le, ge uint32) error {
 	return nil
 }

+func validateImportVRFs(r v1beta1.Router, allVRFs map[string][]string) error {


I suppose this done (curious though if you can run locally in your dev a coverage, I cannot due to the fact that controller unit test needs envtest way of running)

karampok · 2024-07-19T07:27:19Z

api/v1beta1/frrconfiguration_types.go

@@ -79,6 +79,16 @@ type Router struct {
 	// Prefixes is the list of prefixes we want to advertise from this router instance.
 	// +optional
 	Prefixes []string `json:"prefixes,omitempty"`
+
+	// Imports is the list of imports we want for this router / vrf


Imports is the list of imports, could be more readable to like is a list of VRFs from which to import their prefixes?

Here we add the simplest way to route from a VRF to the other, by using import vrf <vrfname>, which is a shortcut to leak routes from a vrf to the other using the vpn SAFI under the default VRF. Also, despite the fact that the import goes in the ipfamily stanza, we choose here to allow an import that includes both the v4 and the v6 family. Signed-off-by: Federico Paolinelli <[email protected]>

We introduce a new importVRFs field to list the vrfs imported to the v4 / v6 AFIs of a given router. Also, we implement the rendering of that field in the frr config. Signed-off-by: Federico Paolinelli <[email protected]>

Translating and merging the new field. Signed-off-by: Federico Paolinelli <[email protected]>

Instead of relying on the returned bool, we propagate the error and we assert on the error routes not found when we know the routes shouldn't be there. This makes triaging easier as the error is more descriptive than a bool. Signed-off-by: Federico Paolinelli <[email protected]>

Here we test leaking routes from / to VRFs with different types of allowed prefixes. Signed-off-by: Federico Paolinelli <[email protected]>

Now that we use EquateEmpty, the expected value can be shorter and more compact, making the tests easier to navigate and handle. Signed-off-by: Federico Paolinelli <[email protected]>

Leaking advertised networks doesn't work in FRR 9.1, so we disable the test. We will re-enable them once the CI is stable with FRR 10. Signed-off-by: Federico Paolinelli <[email protected]>

fedepaol force-pushed the leakvrf branch 7 times, most recently from 22ac9f8 to f74e335 Compare June 28, 2024 12:26

fedepaol changed the title ~~WIP: Implement VRF route leaking~~ Implement VRF route leaking Jun 28, 2024

github-actions bot added the kind/feature label Jun 28, 2024

fedepaol force-pushed the leakvrf branch 7 times, most recently from 2e85fd6 to bee5ac2 Compare June 29, 2024 19:05

karampok reviewed Jul 16, 2024

View reviewed changes

fedepaol force-pushed the leakvrf branch from 9ea9c58 to 06c8959 Compare July 16, 2024 19:08

karampok reviewed Jul 18, 2024

View reviewed changes

fedepaol force-pushed the leakvrf branch from 06c8959 to 6d5f789 Compare July 18, 2024 14:26

karampok reviewed Jul 18, 2024

View reviewed changes

fedepaol force-pushed the leakvrf branch 2 times, most recently from 906e0c6 to d1926fa Compare July 18, 2024 15:16

karampok approved these changes Jul 19, 2024

View reviewed changes

fedepaol force-pushed the leakvrf branch from d1926fa to 862e458 Compare July 19, 2024 07:45

fedepaol added 3 commits July 19, 2024 03:50

FRR: export and render the importVRFs field

6327fa7

We introduce a new importVRFs field to list the vrfs imported to the v4 / v6 AFIs of a given router. Also, we implement the rendering of that field in the frr config. Signed-off-by: Federico Paolinelli <[email protected]>

Controller: reconcile the rendering of imported VRFs

0522207

Translating and merging the new field. Signed-off-by: Federico Paolinelli <[email protected]>

fedepaol added 4 commits July 19, 2024 03:50

E2E: test leaking routes from VRFs

1a3dc65

Here we test leaking routes from / to VRFs with different types of allowed prefixes. Signed-off-by: Federico Paolinelli <[email protected]>

Conversion unit tests: remove the empty fields from expected

99c7bbd

Now that we use EquateEmpty, the expected value can be shorter and more compact, making the tests easier to navigate and handle. Signed-off-by: Federico Paolinelli <[email protected]>

CI: disable the leak / advertising tests

bd942df

Leaking advertised networks doesn't work in FRR 9.1, so we disable the test. We will re-enable them once the CI is stable with FRR 10. Signed-off-by: Federico Paolinelli <[email protected]>

fedepaol force-pushed the leakvrf branch from 862e458 to bd942df Compare July 19, 2024 07:50

karampok approved these changes Jul 19, 2024

View reviewed changes

fedepaol merged commit 30da0a4 into metallb:main Jul 19, 2024
11 checks passed

		ip prefix-list 192.168.1.2-pl-ipv4 seq 1 deny any
		ipv6 prefix-list 192.168.1.2-pl-ipv4 seq 2 deny any


		updateAndCheckPeered(*config, peersDefault, peersVRF, frrsDefault, frrsVRF, ipFamily)

		ginkgo.By("validating")

Implement VRF route leaking #160

Implement VRF route leaking #160

Conversation

fedepaol commented Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol commented Jun 21, 2024 •

edited

Loading

fedepaol Jul 18, 2024 •

edited

Loading